Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate tests that rely on SecurityManager (Java8andUp) #14442

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

EricYangIBM
Copy link
Contributor

SecurityManager will be removed in a future jdk. Separate tests that
rely on it so that they can be easily disabled in the future.

Issue: #14412
Signed-off-by: Eric Yang [email protected]

@EricYangIBM
Copy link
Contributor Author

Added compilation excludes

@pshipton
Copy link
Member

pshipton commented Feb 9, 2022

We don't want to actually exclude the tests for jdk19+ until it's determined the SecurityManager is removed in jdk19. I was just a guess. We want to be ready to do so quickly, so we aren't blocked from running tests for a month when the change happens.

@JasonFengJ9
Copy link
Member

We don't want to actually exclude the tests for jdk19+ until it's determined the SecurityManager is removed in jdk19. I was just a guess. We want to be ready to do so quickly, so we aren't blocked from running tests for a month when the change happens.

I think this PR is preparing us to continue running tests when the SecurityManger related classes are removed in JDK19 or a future release. If it doesn't happen at JDK19, the change required is to just put JDK19 back to the allowed Java levels <matches string="${JDK_VERSION}" pattern="^(8|9|(1[0-8]))$$" /> (one line modification), otherwise this PR will have to be developed in short time period along with quite a few other places.

A list of deprecated APIs for removal:

java.lang.System::{setSecurityManager, getSecurityManager}
java.security.{Policy, PolicySpi, Policy.Parameters}
java.security.{AccessController, AccessControlContext, AccessControlException, DomainCombiner}

java.lang.Thread::checkAccess
java.lang.ThreadGroup::checkAccess
java.util.logging.LogManager::checkAccess
java.util.concurrent.Executors::{privilegedCallable, privilegedCallableUsingCurrentClassLoader, privilegedThreadFactory}
java.rmi.RMISecurityManager
javax.security.auth.SubjectDomainCombiner and javax.security.auth.Subject::{doAsPrivileged, getSubject} 

fyi @EricYangIBM

@pshipton
Copy link
Member

If it doesn't happen at JDK19, the change required is to just put JDK19 back to the allowed Java levels

I can agree with that, as long as we have an issue in the 0.34 milestone plan to ensure we re-enable the tests if necessary. Ideally referring to all the PRs / places to fix. I've created the 0.34 milestone.

<version>15</version>
<version>16</version>
<version>17</version>
<version>18</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will also have to add to versions. (Is there a better way than listing all the versions individually?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renfeiw @llxia do we support something like <version>8-18</version>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we don't, we can have something like this in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have a plan to add something like [8,18]. But we also need to support it in the internal perl tkg as well.

@EricYangIBM EricYangIBM force-pushed the sm_Java8andUp branch 2 times, most recently from c9df40e to 9f49220 Compare February 15, 2022 18:13
@EricYangIBM EricYangIBM marked this pull request as ready for review February 15, 2022 20:52
@JasonFengJ9
Copy link
Member

Are there more changes coming for following tests?

Java8andUp/src/org/openj9/test/annotationPackage/Test_PackageAnnotation.java
Java8andUp/src/org/openj9/test/vmArguments/VmArgumentTests.java
Java8andUp/src_80/org/openj9/test/java/lang/Test_Class.java
Java8andUp/src_90/org/openj9/test/java/lang/Test_Class.java

@EricYangIBM
Copy link
Contributor Author

Java8andUp/src/org/openj9/test/vmArguments/VmArgumentTests.java

Is this related to security manager? -Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy

@JasonFengJ9
Copy link
Member

Is this related to security manager? -Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy

Usually java.security.policy is related to SecurityManager [1].

[1] https://docs.oracle.com/javase/8/docs/technotes/guides/security/PolicyFiles.html

@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Feb 16, 2022

For the two Test_Class.java, since they are src_80 and src_90 won't they not be compiled for jdk19+?
Also for the VmArgumentTests.java I think we can just check the version at runtime and add "-Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy" if the version is less than 19

@JasonFengJ9
Copy link
Member

JasonFengJ9 commented Feb 16, 2022

For the two Test_Class.java, since they are src_80 and src_90

Scratch these two then, on the other hand, it means src_90 isn't used at all since there is no Java 9 build/test.

Also for the VmArgumentTests.java I think we can just check the version at runtime and add "-Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy" if the version is less than 19

Runtime check is fine for this test.

@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Feb 16, 2022

I added the runtime check for VmArgumentTests and removed the AccessController.doPrivileged in Test_PackageAnnotation (SecurityException wasn't thrown, also only used for logging so probably fine)

Note: we will have to change if (VersionCheck.major() < 19) if our assumption about which jdk deprecates SecurityManager is wrong

<exclude name="org/openj9/test/java/lang/invoke/Test_MethodHandleInfo_SM.java" />
<exclude name="org/openj9/test/java/lang/Test_ClassLoader_SM.java" />
<exclude name="org/openj9/test/java/lang/Test_J9VMInternals.java" />
<exclude name="org/openj9/test/java/lang/Test_J9VMInternalsImpl.java" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some sub-tests seem applicable to JDK levels w/o SecurityManager related classes such as TestNCDFE.runTest1() within test_checkPackageAccess().

Copy link
Contributor Author

@EricYangIBM EricYangIBM Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the comment TestNG parses the test file, which interferes with the classloading under test I added the security manager test classes with the same two file pattern now

<version>15</version>
<version>16</version>
<version>17</version>
<version>18</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 11/17/18 are required.

-groups $(TEST_GROUP) \
-excludegroups $(DEFAULT_EXCLUDE); \
$(TEST_STATUS)</command>
<platformRequirements>^vm.hrt</platformRequirements>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vm.hrt discontinued long time ago, this platformRequirements is not needed.

<version>17</version>
<version>18</version>
</versions>
<aot>nonapplicable</aot>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this aot limitation is not needed either.

-groups $(TEST_GROUP) \
-excludegroups $(DEFAULT_EXCLUDE); \
$(TEST_STATUS)</command>
<platformRequirements>^vm.hrt</platformRequirements>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly not required.

<version>15</version>
<version>16</version>
<version>17</version>
<version>18</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only need 11/17/18, assuming this is a SM version of JCL_Test_none_SCC which is for 11+.

<version>15</version>
<version>16</version>
<version>17</version>
<version>18</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 11/17/18 are needed.

-groups $(TEST_GROUP) \
-excludegroups $(DEFAULT_EXCLUDE); \
$(TEST_STATUS)</command>
<platformRequirements>^vm.hrt</platformRequirements>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required.

<version>15</version>
<version>16</version>
<version>17</version>
<version>18</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 11/17/18 are required.

<src path="src_110_up" />
<src path="${src_version}" />
<src path="${src_90_jcl}" />
<src path="${src_access}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script reads better w/o large number of duplicated elements. Can you try something like

  <path id="src.path">
    <pathelement path="${src}/"/>
   ...
    <pathelement path="${src_access}"/>
   ...
  </path>

The src.path can be shared among targets.

<javac ...>
   <sourcepath refid="src.path"/>
   ...
</javac>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to remove duplication of src and class path using a property and path filesets

<compilerarg line='${addExports}' />
<compilerarg line='${addExports_90_jcl}' />
<compilerarg line='${addExports_version}' />
<compilerarg line='--add-modules openj9.sharedclasses' />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to remove duplicated compilerarg w/ something like

<property name="common-args" value="${addExports} ${addExports_90_jcl} ..." />

<compilerarg line="${common-args}" />

* SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception
*******************************************************************************/

import org.testng.annotations.AfterMethod;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not required in this class.

import org.testng.annotations.AfterMethod;
import org.testng.annotations.Test;
import org.testng.log4testng.Logger;
import org.testng.annotations.BeforeMethod;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required.

@Test
public void test_getMethods_subtest2() {
java.security.PrivilegedAction action = new java.security.PrivilegedAction() {
public Object run() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these existing code are being moved to a new class, please help some cleanup.

Suggested change
public Object run() {
PrivilegedAction<Object> action = new PrivilegedAction<Object>() {
@Override
public Object run() {

java.net.URL url = new java.net.URL("file:" + file.getPath());
ClassLoader loader = new java.net.URLClassLoader(new java.net.URL[]{url}, null);

Class cls_E = Class.forName("org.openj9.resources.classinheritance.E", false, loader);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Class cls_E = Class.forName("org.openj9.resources.classinheritance.E", false, loader);
Class<?> cls_E = Class.forName("org.openj9.resources.classinheritance.E", false, loader);

Support_Resources.copyFile(resources, null, "openj9tr_general.jar");
File file = new File(resources.toString() + "/openj9tr_general.jar");
java.net.URL url = new java.net.URL("file:" + file.getPath());
ClassLoader loader = new java.net.URLClassLoader(new java.net.URL[]{url}, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ClassLoader loader = new java.net.URLClassLoader(new java.net.URL[]{url}, null);
ClassLoader loader = new URLClassLoader(new java.net.URL[]{url}, null);

return null;
}
};
java.security.AccessController.doPrivileged(action);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
java.security.AccessController.doPrivileged(action);
AccessController.doPrivileged(action);

};
class MyCombiner implements java.security.DomainCombiner {
boolean combine;
public java.security.ProtectionDomain[] combine(java.security.ProtectionDomain[] executionDomains, java.security.ProtectionDomain[] parentDomains) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public java.security.ProtectionDomain[] combine(java.security.ProtectionDomain[] executionDomains, java.security.ProtectionDomain[] parentDomains) {
public ProtectionDomain[] combine(ProtectionDomain[] executionDomains, ProtectionDomain[] parentDomains) {

*/
@Test
public void test_getClasses2() {
final java.security.Permission privCheckPermission = new java.security.BasicPermission("Privilege check") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
final java.security.Permission privCheckPermission = new java.security.BasicPermission("Privilege check") {
final Permission privCheckPermission = new BasicPermission("Privilege check") {

final MyCombiner combiner = new MyCombiner();
class SecurityManagerCheck extends SecurityManager {
String reason;
Class checkClass;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Class checkClass;
Class<?> checkClass;

Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More cleanup for new classes please.

<exclude name="**/Cmvc194280.java" />
<exclude name="**/resources/**" />
<!-- requires special compilation methods -->
<exclude name="**/NoSuchMethod/**" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These duplicated exclude can be shared via something like

<property name="commonExcludes" value="**/Cmvc194280.java,**/resources/**, ... " />
<javac srcdir="${src}" excludes="${commonExcludes}"


import org.openj9.test.support.resource.Support_Resources;


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking : an extra line.

private static final Logger logger = Logger.getLogger(Test_Class_SM.class);

@Test
public void test_getMethods_subtest2() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking : pls add an indention to align w/ private static final Logger logger.

public Object run() {
try {
File resources = Support_Resources.createTempFolder();
String[] expected_E = new String[] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: use a apace instead of the tab between String[] and expected_E.

Support_Resources.copyFile(resources, null, "openj9tr_general.jar");
File file = new File(resources.toString() + "/openj9tr_general.jar");
URL url = new URL("file:" + file.getPath());
ClassLoader loader = new URLClassLoader(new URL[]{url}, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ClassLoader loader = new URLClassLoader(new URL[]{url}, null);
ClassLoader loader = new URLClassLoader(new URL[] { url }, null);

private Class<?> getClass( String className )throws ClassNotFoundException {
String classFile = className.replace('.', '/') + ".class";
try {
InputStream classStream = getClass().getClassLoader().getResourceAsStream( classFile );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: additional spaces around classFile

String classFile = className.replace('.', '/') + ".class";
try {
InputStream classStream = getClass().getClassLoader().getResourceAsStream( classFile );
if ( classStream == null ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: additional spaces around classStream == null

try {
InputStream classStream = getClass().getClassLoader().getResourceAsStream( classFile );
if ( classStream == null ) {
throw new ClassNotFoundException( "Error loading class : " + classFile );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: additional spaces around "Error loading class : " + classFile

Class<?> clazz = defineClass( className, classRep, 0, classRep.length );
return clazz;
} catch ( IOException e ) {
throw new ClassNotFoundException( e.getMessage() );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new ClassNotFoundException( e.getMessage() );
DataInputStream in = new DataInputStream(classStream);
in.readFully(classRep);
in.close();
Class<?> clazz = defineClass(className, classRep, 0, classRep.length);
return clazz;
} catch (IOException e) {
throw new ClassNotFoundException(e.getMessage());

nitpicking: remove additional spaces, also a few other places in this class.


expectedArgs = new String[initialExpectedArgs.length + 1];
System.arraycopy(initialExpectedArgs, 0, expectedArgs, 0, initialExpectedArgs.length);
expectedArgs[initialExpectedArgs.length] = "-Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls define a string constant to share -Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy, probably -Djava.ext.dirs=/opt/IBM/WebSphere/AppServer80/tivoli/tam:/opt/IBM/WebSphere/AppServer80/java/jre/lib/ext as well.

@JasonFengJ9
Copy link
Member

Requesting reviews from @llxia @pshipton before asking for rebase & squash the commits.

@llxia
Copy link
Contributor

llxia commented Feb 24, 2022

@renfeiw could you help to review this PR? Thanks

@EricYangIBM
Copy link
Contributor Author

I have two other smaller PRs for JLM_Tests and Jsr292 ready: #14562 #14580
@JasonFengJ9 could you please review these as well?

@pshipton
Copy link
Member

This isn't working on vmfarm, see /jobs_by_status.php?build_id=23133&status=FAILED

[TestNG] [ERROR] 
Cannot find class in classpath: org.openj9.test.java.lang.Test_Class_SM
Exception in thread "main" java.lang.NullPointerException
	at org.testng.TestNG.getStatus(TestNG.java:211)
	at org.testng.TestNG.main(TestNG.java:1324)

I didn't try jenkins (yet) but I assume it's been tested there.

@EricYangIBM
Copy link
Contributor Author

I think the problem is that it is looking for Test_Class_SM and Test_System_SM, both only in src_110_up right now. I will need to add duplicates or move them to src.
Also I will need to add the new SM tests to SE80 tests

@EricYangIBM
Copy link
Contributor Author

@JasonFengJ9
Copy link
Member

JavaNext(19) doesn't work.

===============================================
Running test truncatedReturnTest_0 ...
===============================================
truncatedReturnTest_0 Start Time: Fri Feb 25 09:45:22 2022 Epoch Time (ms): 1645811122799
variation: NoOptions
JVM_OPTIONS:  
[TestNG] [ERROR]
Cannot find class in classpath: org.openj9.test.java.lang.Test_ClassLoader_SM
Exception in thread "main" java.lang.NullPointerException: Cannot invoke "org.testng.internal.ExitCodeListener.hasTests()" because "this.exitCodeListener" is null
        at org.testng.TestNG.getStatus(TestNG.java:211)
        at org.testng.TestNG.main(TestNG.java:1324)

truncatedReturnTest_0_FAILED

@pshipton
Copy link
Member

A vmfarm build didn't work either (/build_info.php?build_id=23259), although the problem doesn't seem obviously related to these changes. I'm not sure yet what else it could be.

@JasonFengJ9
Copy link
Member

JasonFengJ9 commented Feb 25, 2022

A vmfarm build didn't work either (/build_info.php?build_id=23259)

    [javac] error: Exception thrown while constructing Processor object: JVMCFRE003 bad major version; class=org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor, offset=6
  [antcall] Exiting /bluebird/builds/bld_23259/testing/com.ibm.j9.jit.fiorano/build.xml.

@pshipton
Copy link
Member

The vmfarm build problem also occurs without these changes so something else has caused it. I'll try these changes again once the other problem is resolved.

@EricYangIBM
Copy link
Contributor Author

Am I correct that all test classes named in testng.xml require a java class? I noticed that the previously excluded test files either were not in testng.xml or they were commented out. I think I'll have to create a separate testng file for the SecurityManager tests

@pshipton
Copy link
Member

A new vmfarm build passed.

@pshipton
Copy link
Member

Although that was before the latest changes. I'll run a new one.

@pshipton
Copy link
Member

Latest changes also passed on vmfarm.

@pshipton
Copy link
Member

pshipton commented Mar 1, 2022

jenkins test sanity xlinux jdk8,jdk11,jdk17,jdk18,jdknext

@pshipton
Copy link
Member

pshipton commented Mar 1, 2022

lgtm, I can see all the new tests running on 11-18, and not on 19.

JCL_Test_SM
JCL_Test_SM_none_SCC

  • JCL_TEST_Java-Lang_ClassLoader_SM
  • JCL_TEST_Java-Lang_SM
  • JCL_TEST_Java-Lang-Invoke_SM
  • JCL_TEST_IBM-VM_SM

JCL_TEST_Java-Security_SM

  • JCL_TEST_Java-Security_SM

J9VMInternals_Test_SM

  • JCL_TEST_Java-Internals_SM

SecurityManager will be removed in a future jdk. Separate tests that
rely on it so that they can be easily disabled in the future.

Issue: eclipse-openj9#14412
Signed-off-by: Eric Yang <[email protected]>
Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks @EricYangIBM

@pshipton
Copy link
Member

pshipton commented Mar 2, 2022

@renfeiw @llxia any further comments?

Copy link
Contributor

@renfeiw renfeiw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pshipton pshipton merged commit afec275 into eclipse-openj9:master Mar 3, 2022
@EricYangIBM EricYangIBM deleted the sm_Java8andUp branch March 3, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants